Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preview Qiskit 12403 #1380

Closed
wants to merge 2 commits into from
Closed

Preview Qiskit 12403 #1380

wants to merge 2 commits into from

Conversation

Eric-Arellano
Copy link
Collaborator

No description provided.

@Eric-Arellano Eric-Arellano deleted the EA/preview-12403 branch May 17, 2024 13:22
github-merge-queue bot pushed a commit that referenced this pull request Jun 10, 2024
Should fix 1034.

## Investigation

It seems PR previews _are_ being cleaned up correctly for _merged_ PRs;
the teardown action is running, and the previews are no longer
accessible after merging (see following screenshot as example).

<img width="300" alt="Screenshot 2024-06-07 at 13 35 48"
src="https://github.com/Qiskit/documentation/assets/36071638/0066b309-9432-4213-81bd-1836521a8900">

It seems the problem is if a PR is closed but not merged (e.g. #1468,
#1380, and #1289 still have active previews). I believe this is the
reason:
https://github.com/orgs/community/discussions/26657#discussioncomment-3252753.


## Solution

This PR tries to fix this by using the `pull_request_target` event to
trigger teardowns. This should trigger correctly when PRs are closed.

We need to be careful with this event as it allows untrusted PRs to
trigger events using secrets. Since this action runs on the target
branch (rather than a merge commit of the target and PR branches), an
untrusted user shouldn't be able to do anything malicious, but we need
to make sure we **never** checkout the PR branch with this event type,
or read any user-defined inputs (such as the PR title) as it could lead
to an injection. I think this is unlikely, but I've left a warning in
the action just in case.

See frankharkins#7 for a
demonstration. The "Preview teardown" step ran after the PR was closed.
@frankharkins frankharkins restored the EA/preview-12403 branch June 10, 2024 14:34
@frankharkins frankharkins reopened this Jun 10, 2024
@frankharkins frankharkins deleted the EA/preview-12403 branch June 10, 2024 14:35
frankharkins added a commit to frankharkins/documentation that referenced this pull request Jul 22, 2024
Should fix 1034.

## Investigation

It seems PR previews _are_ being cleaned up correctly for _merged_ PRs;
the teardown action is running, and the previews are no longer
accessible after merging (see following screenshot as example).

<img width="300" alt="Screenshot 2024-06-07 at 13 35 48"
src="https://github.com/Qiskit/documentation/assets/36071638/0066b309-9432-4213-81bd-1836521a8900">

It seems the problem is if a PR is closed but not merged (e.g. Qiskit#1468,
Qiskit#1380, and Qiskit#1289 still have active previews). I believe this is the
reason:
https://github.com/orgs/community/discussions/26657#discussioncomment-3252753.


## Solution

This PR tries to fix this by using the `pull_request_target` event to
trigger teardowns. This should trigger correctly when PRs are closed.

We need to be careful with this event as it allows untrusted PRs to
trigger events using secrets. Since this action runs on the target
branch (rather than a merge commit of the target and PR branches), an
untrusted user shouldn't be able to do anything malicious, but we need
to make sure we **never** checkout the PR branch with this event type,
or read any user-defined inputs (such as the PR title) as it could lead
to an injection. I think this is unlikely, but I've left a warning in
the action just in case.

See #7 for a
demonstration. The "Preview teardown" step ran after the PR was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants